feat(datasets): add Apache Iceberg dataset support for polars - #1503
Saurav-Gupta-9741 wants to merge 1 commit into
Conversation
a4a0341 to
7297b1d
Compare
eaff45d to
c0a3d8a
Compare
|
@Saurav-Gupta-9741 please reduce this to just one library--which library are you primarily working with? @SajidAlamQB @ankatiyar @ravi-kumar-pilla I would prefer to align on the approach in #1030 first; generating this code is cheap, but I don't think the tradeoffs are fully thought through. (@Saurav-Gupta-9741 would have been preferable that you didn't just jump the gun on this.) |
c0a3d8a to
b1d036a
Compare
|
Hi @deepyaman Sir |
|
Hi @Saurav-Gupta-9741 , Thanks for putting this together, and for narrowing it down to Polars after Deepyaman’s feedback. The structure looks good, CI is green, and it is clear you have thought about catalog config, write modes, and optional dependencies. Before we merge though, I want to align with the direction Deepyaman raised in #1030. I think the goal should be a thin Kedro wrapper around Polars native Iceberg APIs, not a PyIceberg shim inside Right now Load: delegate to Save: delegate to That keeps filter pushdown, streaming, and future Polars Iceberg improvements on the native path instead of maintaining a parallel PyIceberg layer in Kedro. On the issue you concluded that engine native delegation (scan_iceberg, to_iceberg, sink_iceberg) is the right approach. The current PR goes back to a PyIceberg direct implementation instead. Can you explain what changed? If you hit a blocker with the native APIs (version requirements, catalog config, write behavior, etc.) please share that here. If not, I think we should refactor to match what you proposed on #1030. A few other things to tidy up regardless of the above:
Let me know if refactoring to native Polars APIs raises any concerns on your side. Thanks again for the contribution. |
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
Left some comments here - #1503 (comment)
ec8ce13 to
5119f33
Compare
|
Hi @ravi-kumar-pilla Sir, Thank you so much for the detailed and thoughtful review! I have addressed all your points and updated the PR accordingly: 1. Polars Native Delegation (
|
|
Could you also update the PR description to just focus on the one dataset? |
2f4b66a to
06eb459
Compare
ravi-kumar-pilla
left a comment
There was a problem hiding this comment.
Thanks for the updates. Polars only scope, credentials handling, _exists() behaviour, and the PR description all look good.
Two things to fix before merge:
- Module and class docstrings overstate Polars native coverage. Save goes through PyIceberg, not Polars write APIs.
load_argsmixes Polars and PyIceberg scan options in one dict. Invalid keys on the native path get silently swallowed.
| @@ -0,0 +1,204 @@ | |||
| """``IcebergDataset`` loads and saves data from/to Apache Iceberg tables | |||
There was a problem hiding this comment.
The module and class docstrings say load and save use Polars native APIs. That is only partly true.
Load tries scan_iceberg but falls back to PyIceberg. Save does not use write_iceberg or sink_iceberg today. It goes through PyIceberg directly.
Please update both docstrings. Something like:
"""``IcebergDataset`` loads and saves Apache Iceberg tables.
Loads via Polars ``scan_iceberg`` when available, with a PyIceberg scan fallback.
Saves commit data to Iceberg tables. Catalog access is handled through PyIceberg.
"""Adjust the save line depending on whether you keep the direct PyIceberg path or switch to write_iceberg (see comment on _save below).
There was a problem hiding this comment.
Updated both module and class docstrings to accurately reflect: loads via scan_iceberg with PyIceberg fallback, saves via PyIceberg catalog operations.
| credentials: Authentication credentials or secrets (e.g. tokens, AWS/GCP keys). | ||
| These are merged with ``catalog_properties`` when connecting to the catalog | ||
| and are safely excluded from ``_describe()`` to avoid leaking secrets. | ||
| load_args: Additional scan/read options passed to ``polars.scan_iceberg`` |
There was a problem hiding this comment.
load_args documents options for two different APIs in one dict.
polars.scan_iceberg accepts snapshot_id, storage_options, reader_override, etc. PyIceberg table.scan accepts selected_fields, row_filter, limit. These are not interchangeable.
If a user passes selected_fields and native scan is available, Polars raises TypeError. That gets caught by the bare except Exception in _load() and silently falls back to PyIceberg.
I would restrict load_args to Polars native keys only. Alternatively split into separate args or filter keys before calling scan_iceberg.
There was a problem hiding this comment.
Scoped load_args docstring to Polars native scan_iceberg arguments only. In the fallback path, only compatible keys (e.g. snapshot_id) are forwarded to table.scan().
| try: | ||
| lazy_df = pl.scan_iceberg(table, **self._load_args) | ||
| return lazy_df.collect() | ||
| except Exception: # noqa: BLE001 |
There was a problem hiding this comment.
The bare except Exception: pass in _load() can hide real errors. Bad snapshot_id, auth failures, and invalid load_args all get swallowed.
Please narrow the catch to exceptions you genuinely expect during fallback. Re raise on TypeError and ValueError from bad user args.
There was a problem hiding this comment.
Narrowed to except (NotImplementedError, AttributeError). TypeError/ValueError from bad user args now propagate directly. Added test_load_invalid_load_args_raises_type_error to verify.
| arrow_table = scan.to_arrow() | ||
| return pl.from_arrow(arrow_table) | ||
|
|
||
| def _save(self, data: pl.DataFrame | pl.LazyFrame) -> None: |
There was a problem hiding this comment.
For save, consider data.write_iceberg(table, mode=mode) for consistency with Polars' Iceberg API surface. Under the hood it is still PyIceberg, so keeping the current direct path is also fine, especially since write_iceberg is marked unstable.
If you keep the current approach, a short comment in _save() explaining why would help. Update the docstring save line to match whichever approach you go with.
There was a problem hiding this comment.
Added inline comment explaining why we use PyIceberg's Arrow interface directly rather than unstable write_iceberg/sink_iceberg.
| Args: | ||
| table_name: Table identifier (e.g. ``"namespace.table_name"`` or ``"table_name"``). | ||
| catalog_name: Name of the Iceberg catalog to load. Defaults to None. | ||
| catalog_properties: Properties required to instantiate the catalog (e.g. |
There was a problem hiding this comment.
credentials are correctly excluded from _describe(). Secrets in catalog_properties will still show up in _describe(). Worth a note on the catalog_properties arg: do not put secrets here, use credentials instead.
There was a problem hiding this comment.
Added note: "Do not pass secrets/tokens here as they are visible in _describe(); use credentials instead."
| @@ -0,0 +1,208 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Mock tests are a good start. No explicit test for the native scan_iceberg path. test_load_pyiceberg_scan may hit the fallback depending on Polars version.
PR description says 12 tests, file has 13.
There was a problem hiding this comment.
Added test_load_native_scan_iceberg, test_load_pyiceberg_scan_fallback, test_load_invalid_load_args_raises_type_error, and test_missing_polars_raises_error. Total: 16 tests.
| --ignore kedro_datasets/huggingface/transformer_pipeline_dataset.py \ | ||
| --ignore kedro_datasets/pandas/gbq_dataset.py \ | ||
| --ignore kedro_datasets/partitions/partitioned_dataset.py \ | ||
| --ignore kedro_datasets/polars/iceberg_dataset.py \ |
There was a problem hiding this comment.
Please add a short comment on the iceberg_dataset.py ignore entry explaining why (optional deps not available in doctest env).
There was a problem hiding this comment.
Updated comment to mention optional dependencies (pyiceberg) as reason for the ignore.
723a4a1 to
e01074f
Compare
e01074f to
e4acf79
Compare
| # Fall back to PyIceberg scan if native scan is not supported for this table format/engine | ||
| pass | ||
|
|
||
| # Fallback: PyIceberg scan -> to_polars or Arrow zero-copy |
There was a problem hiding this comment.
I don't like this. Datasets are simple wrappers; set the bound for when scan_iceberg was introduced in the dependencies; if the user has an older version, they can't leverage the dataset, and that's fine.
There was a problem hiding this comment.
Agreed — removed the fallback entirely. _load() is now a single line: pl.scan_iceberg(table, **self._load_args).collect().
| arrow_table = scan.to_arrow() | ||
| return pl.from_arrow(arrow_table) | ||
|
|
||
| def _save(self, data: pl.DataFrame | pl.LazyFrame) -> None: |
There was a problem hiding this comment.
Removed along with the entire fallback path.
|
|
||
| def _save(self, data: pl.DataFrame | pl.LazyFrame) -> None: | ||
| """Saves a Polars DataFrame into the Apache Iceberg table.""" | ||
| if hasattr(data, "collect"): |
There was a problem hiding this comment.
Do other Polars datasets handle both lazy and eager frames like this?
There was a problem hiding this comment.
Updated to accept only pl.DataFrame, consistent with EagerPolarsDataset. Removed the hasattr(data, "collect") check.
| catalog = self._get_catalog() | ||
| arrow_table = data.to_arrow() | ||
|
|
||
| # We write via PyIceberg's Arrow interface (append / overwrite) directly |
There was a problem hiding this comment.
Disagree, same reason as above. If you don't feel the sink_iceberg/write_iceberg are ready for use, that should have come out of the discussion on the issue. The Polars Iceberg dataset defers to Polars.
There was a problem hiding this comment.
Refactored to use data.write_iceberg(table, mode=mode). Removed all PyIceberg direct write logic and table auto-creation.
| fi; \ | ||
| \ | ||
| # The ignored datasets below require complicated setup with cloud/database clients or network model download which is overkill for the doctest examples. | ||
| # The ignored datasets below require complicated setup with cloud/database clients, optional dependencies (e.g. pyiceberg for iceberg_dataset), or network model download which is overkill for the doctest examples. |
There was a problem hiding this comment.
We don't exclude things from doctests due to optional dependencies? I don't understand what makes it hard for this to work.
There was a problem hiding this comment.
Removed the --ignore kedro_datasets/polars/iceberg_dataset.py entry and reverted the comment to its original text. Docstring examples already use # doctest: +SKIP.
Add polars.IcebergDataset for reading/writing Apache Iceberg tables via Polars native scan_iceberg with PyIceberg catalog fallback and secure credentials handling. Closes kedro-org#1030 Signed-off-by: Saurav Gupta <91198524+Saurav-Gupta-13@users.noreply.github.com>
e4acf79 to
4d7453e
Compare
Description & Motivation
Closes #1030.
This PR introduces native Apache Iceberg dataset support for Polars in
kedro-datasets:polars.IcebergDataset(kedro_datasets.polars.IcebergDataset)Key Capabilities:
polars.scan_iceberg(table, **load_args).collect()for native filter pushdown, projection, and streaming benefits, with graceful zero-copy Arrow fallback via PyIceberg.pyiceberg.catalog.load_catalog().credentialsparameter for catalog and storage authentication (e.g. tokens, access keys) while safely excluding credentials from_describe().overwrite(default) andappendcommit modes for eagerpl.DataFrameandpl.LazyFrame.How Has This Been Tested?
_describe()_exists()(NoSuchTableError,NoSuchNamespaceError)ruff.Checklist
ruff check,ruff format)RELEASE.md